feat(skin): add margin skin (11 themes; light/dark per theme; switchable from Settings)#7880
Closed
deepshekhardas wants to merge 1 commit into
Closed
feat(skin): add margin skin (11 themes; light/dark per theme; switchable from Settings)#7880deepshekhardas wants to merge 1 commit into
deepshekhardas wants to merge 1 commit into
Conversation
…ble from Settings) Upstream PR: ether#7721
Review Summary by Qodo(Agentic_describe updated until commit 25a7158)Add margin skin with 6 themes and light/dark mode support
WalkthroughsDescription• Adds a new margin skin with 6 selectable themes (Colibris, Editorial, Brutalist, Paper, CRT, Industrial) and light/dark mode toggle • Implements theme and dark mode management across pad, lobby, and timeslider pages with early application to prevent flash of unstyled content • Provides complete CSS variable system for all 11 theme combinations with theme-specific visual touches (italic styling, brutalist borders, paper rounded corners, CRT scanlines and glow effects, Industrial accent underlines) • Builds theme selector dropdown and dark mode toggle UI elements injected into user-settings and pad-settings sections • Propagates theme attributes into nested editor iframes (ace_outer and ace_inner) for visual consistency • Implements recent pads list functionality on lobby page with deduplication, sorting by timestamp, and formatted display with member counts • Includes comprehensive styling for all UI components (toolbar, buttons, forms, popups, chat, comments, tables2 plugin, color picker, etc.) with theme-aware colors • Adds scrollbar, layout, and responsive design support for various viewport sizes • Includes complete documentation for installation, configuration, and runtime theme switching Diagramflowchart LR
A["User Settings"] -->|"Select Theme"| B["applyMarginTheme()"]
A -->|"Toggle Dark Mode"| C["applyMarginMode()"]
B -->|"Set data-theme"| D["Document Root"]
C -->|"Set data-mode"| D
D -->|"Apply CSS Variables"| E["Pad/Lobby/Timeslider"]
D -->|"Propagate to iframes"| F["Editor Iframes"]
G["localStorage"] -->|"Restore Preferences"| B
G -->|"Restore Preferences"| C
H["Recent Pads List"] -->|"Display with Theme"| I["Lobby Page"]
File Changes1. src/static/skins/margin/pad.js
|
Code Review by Qodo
1. Non-UTF8 skin files
|
Code Review by Qodo
1. Recent pad links break
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+1
to
+3
| # margin GÇö Etherpad skin | ||
|
|
||
| A standalone drop-in skin with six themes and an orthogonal Light/Dark toggle: |
There was a problem hiding this comment.
1. Non-utf8 skin files 🐞 Bug ☼ Reliability
Several newly added margin skin assets contain invalid UTF-8 byte sequences (visible as mojibake like "G��" / "Sekund+�r"), which can break tooling that reads these files as UTF-8 and risks serving garbled text to browsers. This needs re-encoding/cleanup before merge to avoid brittle builds and hard-to-debug asset issues.
Agent Prompt
### Issue description
Multiple newly added `margin` skin files contain invalid UTF-8 bytes (mojibake sequences like `G��` / `+�`), which breaks UTF-8 decoding in tooling and can result in corrupted comments/text in served assets.
### Issue Context
These files are static assets (CSS/JS/MD) that are expected to be UTF-8 in typical web/build pipelines.
### Fix Focus Areas
- src/static/skins/margin/README.md[1-61]
- src/static/skins/margin/index.css[1-43]
- src/static/skins/margin/pad.css[1-416]
- src/static/skins/margin/pad.js[1-253]
- src/static/skins/margin/timeslider.js[1-42]
- src/static/skins/margin/src/pad-editor.css[1-48]
- src/static/skins/margin/src/components/buttons.css[1-74]
### What to change
- Convert the listed files to valid UTF-8.
- Replace any corrupted punctuation/characters in comments (or remove them) so the repository contains clean UTF-8 sources.
- Re-run whatever formatter/linter/build step is used for static assets to ensure no further encoding regressions.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+104
to
+113
| // Use new URL() so a trailing slash, query string, or hash on | ||
| // window.location.href doesn't produce a broken link, and so pad | ||
| // names with characters that need encoding still resolve. | ||
| const padPath = new URL(`p/${encodeURIComponent(pad.name)}`, | ||
| window.location.href).href; | ||
| const link = document.createElement('a'); | ||
| link.style.textDecoration = 'none'; | ||
|
|
||
| link.href = padPath; | ||
| link.innerText = pad.name; |
There was a problem hiding this comment.
2. Recent pad link broken 🐞 Bug ≡ Correctness
margin/pad.js stores the pad name directly from window.location.pathname (percent-encoded), but margin/index.js applies encodeURIComponent() again when building the recent-pad URL, which double-encodes and breaks navigation for pad names containing spaces or non-ASCII characters. Users will click a recent pad and be sent to a different (wrong) pad URL (e.g., %20 becomes %2520).
Agent Prompt
### Issue description
Recent pad links can be double-encoded: the stored pad name is derived from `window.location.pathname` (already percent-encoded) and later encoded again with `encodeURIComponent()` when generating the link.
### Issue Context
Core navigation encodes pad names when generating `/p/<pad>` URLs, so the pathname segment can contain `%..` sequences.
### Fix Focus Areas
- src/static/skins/margin/pad.js[218-249]
- src/static/skins/margin/index.js[104-113]
### What to change
Choose ONE consistent representation for `recentPads[].name`:
1) **Store decoded names**: in `pad.js`, set `padName = decodeURIComponent(lastSegment)` before saving. Keep `encodeURIComponent(pad.name)` in `index.js`.
OR
2) **Store encoded names**: keep current storage, but in `index.js` remove the extra encoding and treat `pad.name` as already encoded when building the URL.
Also consider displaying the decoded name to users (so they don’t see `%20` in the list).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+60
to
+147
| try { | ||
| const recentPadsFromLocalStorage = localStorage.getItem('recentPads'); | ||
| if (recentPadsFromLocalStorage != null) { | ||
| const parsed = JSON.parse(recentPadsFromLocalStorage); | ||
| if (Array.isArray(parsed)) { | ||
| recentPadListData = parsed.filter( | ||
| (p) => p && typeof p === 'object' && typeof p.name === 'string'); | ||
| } | ||
| } | ||
| } catch (_) { /* private mode / corrupted entry */ } | ||
|
|
||
| // Remove duplicates based on pad name and sort by timestamp | ||
| recentPadListData = recentPadListData.filter( | ||
| (pad, index, self) => index === self.findIndex((p) => p.name === pad.name) | ||
| ).sort((a, b) => new Date(a.timestamp) > new Date(b.timestamp) ? -1 : 1); | ||
|
|
||
| if (recentPadList && recentPadListData.length === 0) { | ||
| const parentStyle = recentPadList.parentElement.style; | ||
| recentPadListHeading.setAttribute('data-l10n-id', 'index.recentPadsEmpty'); | ||
| parentStyle.display = 'flex'; | ||
| parentStyle.justifyContent = 'center'; | ||
| parentStyle.alignItems = 'center'; | ||
| parentStyle.maxHeight = '100%'; | ||
| recentPadList.remove(); | ||
| } else if (recentPadList) { | ||
| /** | ||
| * @typedef {Object} Pad | ||
| * @property {string} name | ||
| */ | ||
|
|
||
| /** | ||
| * @param {Pad} pad | ||
| */ | ||
|
|
||
| const arrowIcon = '<svg xmlns="http://www.w3.org/2000/svg" width="24" height="24" viewBox="0 0 24 24" fill="none" stroke="currentColor" stroke-width="2" stroke-linecap="round" stroke-linejoin="round" class="lucide lucide-arrow-right w-4 h-4 text-gray-400"><path d="M5 12h14"></path><path d="m12 5 7 7-7 7"></path></svg>'; | ||
| const clockIcon = '<svg xmlns="http://www.w3.org/2000/svg" width="16" height="16" viewBox="0 0 24 24" fill="none" stroke="currentColor" stroke-width="2" stroke-linecap="round" stroke-linejoin="round" class="lucide lucide-clock w-3 h-3"><circle cx="12" cy="12" r="10"></circle><polyline points="12 6 12 12 16 14"></polyline></svg>'; | ||
| const personalIcon = '<svg xmlns="http://www.w3.org/2000/svg" width="16" height="16" viewBox="0 0 24 24" fill="none" stroke="currentColor" stroke-width="2" stroke-linecap="round" stroke-linejoin="round" class="lucide lucide-users w-3 h-3"><path d="M16 21v-2a4 4 0 0 0-4-4H6a4 4 0 0 0-4 4v2"></path><circle cx="9" cy="7" r="4"></circle><path d="M22 21v-2a4 4 0 0 0-3-3.87"></path><path d="M16 3.13a4 4 0 0 1 0 7.75"></path></svg>'; | ||
| recentPadListData.forEach((pad) => { | ||
| const li = document.createElement('li'); | ||
|
|
||
|
|
||
| li.style.cursor = 'pointer'; | ||
|
|
||
| li.className = 'recent-pad'; | ||
| // Use new URL() so a trailing slash, query string, or hash on | ||
| // window.location.href doesn't produce a broken link, and so pad | ||
| // names with characters that need encoding still resolve. | ||
| const padPath = new URL(`p/${encodeURIComponent(pad.name)}`, | ||
| window.location.href).href; | ||
| const link = document.createElement('a'); | ||
| link.style.textDecoration = 'none'; | ||
|
|
||
| link.href = padPath; | ||
| link.innerText = pad.name; | ||
| li.appendChild(link); | ||
|
|
||
|
|
||
| const arrowIconElement = document.createElement('span'); | ||
| arrowIconElement.className = 'recent-pad-arrow'; | ||
| arrowIconElement.innerHTML = arrowIcon; | ||
| li.appendChild(arrowIconElement); | ||
|
|
||
| const nextRow = document.createElement('div'); | ||
|
|
||
| nextRow.style.display = 'flex'; | ||
| nextRow.style.gap = '10px'; | ||
| nextRow.style.marginTop = '10px'; | ||
|
|
||
| const clockIconElement = document.createElement('span'); | ||
| clockIconElement.className = 'recent-pad-clock'; | ||
| clockIconElement.innerHTML = clockIcon; | ||
|
|
||
| nextRow.appendChild(clockIconElement); | ||
|
|
||
| const time = new Date(pad.timestamp); | ||
| const userLocale = navigator.language || 'en-US'; | ||
|
|
||
| const formattedTime = time.toLocaleDateString(userLocale, { | ||
| year: 'numeric', | ||
| month: '2-digit', | ||
| day: '2-digit', | ||
| hour: '2-digit', | ||
| minute: '2-digit', | ||
| }); | ||
| const timeElement = document.createElement('span'); | ||
| timeElement.className = 'recent-pad-time'; | ||
| timeElement.innerText = formattedTime; | ||
|
|
There was a problem hiding this comment.
3. Invalid timestamp crashes lobby 🐞 Bug ☼ Reliability
margin/index.js filters recentPads entries only by name and then unconditionally formats `new Date(pad.timestamp) via toLocaleDateString(), which throws if timestamp` is missing or invalid. A single malformed entry can abort recent-pad rendering and break the lobby’s recent pads UI.
Agent Prompt
### Issue description
`recentPads` entries are only validated for `name`, but `timestamp` is used for sorting and formatting without validation. Invalid timestamps can throw when calling `toLocaleDateString()`.
### Issue Context
The code explicitly tries to tolerate corrupted `localStorage` entries, but it does not validate `timestamp`.
### Fix Focus Areas
- src/static/skins/margin/index.js[60-75]
- src/static/skins/margin/index.js[134-147]
### What to change
- When parsing `recentPads`, additionally validate `timestamp` (and optionally `members`).
- Example: keep only entries where `typeof p.timestamp === 'string'` and `!Number.isNaN(Date.parse(p.timestamp))`.
- In rendering, guard formatting:
- If `Date.parse()` is NaN, either skip the date, show a placeholder, or fall back to `''`.
- In the sort comparator, handle invalid dates deterministically (e.g., treat invalid as oldest).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
![qodo-free-for-open-source-projects[bot]](https://avatars.githubusercontent.com/in/1068000?s=60&v=4){kind=small}
Comment on lines
+104
to
+108
| // Use new URL() so a trailing slash, query string, or hash on | ||
| // window.location.href doesn't produce a broken link, and so pad | ||
| // names with characters that need encoding still resolve. | ||
| const padPath = new URL(`p/${encodeURIComponent(pad.name)}`, | ||
| window.location.href).href; |
There was a problem hiding this comment.
1. Recent pad links break 🐞 Bug ≡ Correctness
margin/pad.js stores the pad name directly from window.location.pathname (already percent-encoded), but margin/index.js builds recent-pad URLs using encodeURIComponent(pad.name), causing double-encoding (e.g., my%20pad → my%2520pad) and broken navigation from the lobby’s recent list.
Agent Prompt
## Issue description
Recent pad links in the lobby can become invalid because pad names are persisted in `recentPads` in an already-encoded form (from `location.pathname`), then encoded again in `index.js` when constructing the URL.
## Issue Context
- `pad.js` stores the last path segment from `location.pathname` as the pad name.
- Browsers keep `location.pathname` percent-encoded.
- `index.js` encodes `pad.name` again via `encodeURIComponent()`.
## Fix Focus Areas
- src/static/skins/margin/pad.js[218-252]
- src/static/skins/margin/index.js[104-114]
## Implementation notes
- When capturing the pad name in `pad.js`, safely decode it before storing (wrap `decodeURIComponent()` in try/catch).
- When rendering the recent list in `index.js`, treat stored names as decoded and apply `encodeURIComponent()` exactly once when building the URL.
- Consider also displaying the decoded name in the UI (so users don’t see `%20`).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Port of #7721
Rebased onto latest develop for mergeability.